Skip to content

Use fetch instead of got in notion-client#312

Closed
remorses wants to merge 2 commits intoNotionX:masterfrom
remorses:use-fetch
Closed

Use fetch instead of got in notion-client#312
remorses wants to merge 2 commits intoNotionX:masterfrom
remorses:use-fetch

Conversation

@remorses
Copy link
Copy Markdown
Contributor

@remorses remorses commented Jun 27, 2022

Description

Using fetch from undici or the native fetch implementation, this makes it possible to use notion-client in cloudflare workers or other environments other than node

Given that this is a breaking change (renamed gotOptions to fetchOptions) it could need a major version bump, tell me if you want me to change the version of the packages

Fix #160

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-notion-x ✅ Ready (Inspect) Visit Preview Jun 27, 2022 at 0:58AM (UTC)
react-notion-x-minimal-demo ✅ Ready (Inspect) Visit Preview Jun 27, 2022 at 0:58AM (UTC)

@remorses remorses changed the title Sse fetch instead of got in notion-client Use fetch instead of got in notion-client Jun 27, 2022
@pbteja1998
Copy link
Copy Markdown
Contributor

Yes, even I use this cloudflare workers and i had to add a patch everytime to replace got with fetch.

@transitive-bullshit
Copy link
Copy Markdown
Member

Really great PR @remorses. Much appreciated 🙏

My main hesitancy here is that the notion API can be pretty flaky at times, and got includes some really nice, non-trivial retry logic by default. I'm worried that existing use cases would become less stable with this change.

Maybe we could allow for the user to pass in their own fetch implementation to the client constructor?

I'd still like got to be usable without any additional work on the user's part. Maybe we could move it to an optional peer dependency and try conditionally importing it if it's available? If not, then we fallback to the fetch logic in this PR?

@remorses
Copy link
Copy Markdown
Contributor Author

remorses commented Jul 3, 2022

What if we move the retry logic inside the fetch method? The user could then pass something like fetchOptions: {retry: 3}

@transitive-bullshit
Copy link
Copy Markdown
Member

I've finally had a chance to revisit this while trying to get notion-client to work with the new Vercel edge runtime.

The main issue I'm running into is that including node-fetch anywhere in the bundle (even conditionally imported) means that it doesn't work on non-node.js runtimes such as Vercel's edge runtime. I haven't tried with CF workers, but if it works there, then I'd guess the difference is Vercel's eager bundling optimizations.

Either way, I'd love to support this use case for notion-client, but am not sure of how to proceed aside from having the user pass in a fetch implementation which is kind of poor DX.

@transitive-bullshit
Copy link
Copy Markdown
Member

Resolved in #394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Alternative to notion-client format?

3 participants